Repository navigation
fix(Toast): a toast plays its entrance once - #10086
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Toast | 64.5 kB (+1 B) · 9.1 kB CSS (+14 B) | 1,574 (+31) | — · — · — |
Package size: @astryxdesign/core 6.61 MB packed (+978 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 23 rows; run pnpm api:report.
Wide change (shared styles: packages/core/src/theme/derivedVarRegistry.test.ts): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so once it is marked ready for review, a maintainer adds the ci:run label so CI runs.
The fix restores component:Toast FR5 and FR12: the new browser spec's three move cases fail on main and pass on this branch, and the Drawer and bottom-sheet stories show the same before and after.
Two optional comment suggestions are inline on ToastViewport.tsx (the hasEntered name and the transitionrun listener note).
[Automated review]
| // The row's entry has started. The viewport moves between modal outlets, and | ||
| // each move renders the row again, so its starting style must not apply. | ||
| const [hasEntered, setHasEntered] = useState(false); |
There was a problem hiding this comment.
Optional (comment style): the first sentence restates the flag, and transitionrun sets it when the entry starts, so hasEntered says more than it holds. Consider hasEntryStarted and keeping only the reason:
// Each move between modal outlets renders the row again. Its starting style
// must not apply then.
const [hasEntryStarted, setHasEntryStarted] = useState(false);There was a problem hiding this comment.
Done at dbfdaa1: the state is hasEntryStarted, and its comment keeps only the reason.
| // React passes `transitionrun` without its property name, so the row | ||
| // listens for it on the element. |
There was a problem hiding this comment.
Optional: React 19 does deliver this event as onTransitionRun; only its synthetic event lacks propertyName, and event.nativeEvent.propertyName carries it. A handler next to the existing onTransitionEnd could replace the native listener and this comment. If the listener stays, the quirk reads more exactly as:
// React's onTransitionRun event has no propertyName field.There was a problem hiding this comment.
Done at dbfdaa1: the comment now reads as you suggest. The native listener stays: onTransitionRun would need a disable for @eslint-react/dom-no-unknown-property, which flags it.
ac91adb to
dbfdaa1
Compare
There was a problem hiding this comment.
Changes requested. The fix stops the replayed entrance for a toast already on screen when a Dialog, a modal Drawer or a scrim BottomSheet opens or closes, but two issues need fixing first:
- Miss: a toast whose viewport moves during its own 175 ms entrance never settles, because the move cancels the row's
grid-template-rowstransition andisSettledis set only ontransitionend, so its wrapper keepsoverflow: hiddenand the card's shadow stays cut off for as long as the toast shows (ToastViewport.tsx:299,:321). - Miss: the row's new
transitionrunlistener,hasEntryStartedflag and two private start properties duplicate the animation carry thatLayer/preserveAnimations.tsalready owns, so extendpreserveAnimations(it currently loses a first entrance whose transition was cancelled before it started) and call it once around the host move inplace(), or say in the pull request why it cannot serve this move.
Either fix should come with a check that a row moved during its entrance settles, which the two move-during-entrance cases in the new spec do not assert today. CI has not run on this head yet: once the pull request is ready, a maintainer adds the ci:run label so CI runs.
[Automated review]
| data-toast-id={entry.id} | ||
| {...stylex.props( | ||
| styles.toastWrapper, | ||
| !hasEntryStarted && styles.toastWrapperEntering, |
There was a problem hiding this comment.
Miss: once this drops the starting style, a host move during the entrance cancels the row's grid-template-rows transition (transitioncancel) and starts none, so onTransitionEnd never sets isSettled and the inner wrapper keeps overflow: hidden: the card's shadow stays cut off until the toast leaves. If the local approach stays, settling the row on that cancel would cover it, for example inside watchEntry:
const onCancel = (event: TransitionEvent) => {
if (event.target === row && event.propertyName === 'grid-template-rows') {
// A move cancels the entry; the row then renders open with no transition.
setIsSettled(true);
}
};
row.addEventListener('transitioncancel', onCancel);The "enters once" and "…when a Dialog opens over it" cases both move the row during its entrance, so a check that the row settles there (for example the inner wrapper's computed overflow) would catch this.
There was a problem hiding this comment.
Fixed at 38b1799: the row settles on its own transitioncancel for grid-template-rows. The move-during-entrance case now slows the entrance and checks that the clip turns visible; it fails without the fix in both engines.
| setHasEntryStarted(true); | ||
| } | ||
| }; | ||
| row.addEventListener('transitionrun', onRun); |
There was a problem hiding this comment.
Miss (one shared path): Layer/preserveAnimations.ts already carries a layer's CSS transitions across a reset and finishes a recreated @starting-style transition. Wrapping the host move in place() with preserveAnimations(host, () => …) keeps a shown toast still in the Dialog close and open cases and releases the clip, but it loses the first entrance of a toast raised in the same task as the close, so the shared owner needs that one extension. Extending it and calling it once from place() would replace this listener, hasEntryStarted and the two private start properties; if it cannot serve the host move, please say why in the description.
There was a problem hiding this comment.
Kept local, and the description now says why. preserveAnimations cannot tell a toast whose entrance has not started from one that settled long ago, since neither has an animation to save. And its resumed script animation fires no transitionend, which the row's settle and exit wait for.
dbfdaa1 to
38b1799
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so once it is ready for review, a maintainer adds the ci:run label so CI runs.
The fix restores component:Toast FR5 and FR12: the new browser spec's three move cases fail on main and pass on this branch, a toast moved during its entrance now settles with its shadow, and the description explains why the shared animation carry cannot serve this move.
One optional comment-wording suggestion is inline on ToastViewport.tsx.
[Automated review]
| // The card's starting style reads these. A card that has entered then | ||
| // starts where it is when a move renders it again. |
There was a problem hiding this comment.
Optional (comment wording): these properties are set when the entrance starts, and a card moved during its entrance starts at rest rather than where it is. Consider:
// The card's starting style reads these. Once the entrance starts, a move
// renders the card at rest.There was a problem hiding this comment.
Done at 797b0b3: the comment now reads as you suggest, and the commit message and description say "at rest" too.
38b1799 to
797b0b3
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The pull request is a draft from a fork, so once it is ready for review, a maintainer adds the ci:run label so CI runs.
The fix restores component:Toast FR5 and FR12: the new browser spec's three move cases fail on main and pass on this branch, also when merged with current main, and a Dialog, modal Drawer or scrim BottomSheet opening or closing over a toast no longer replays its entrance.
One optional wording suggestion for the two private doc entries is inline on Toast.doc.mjs.
[Automated review]
| 'Private starting opacity of the entry; ToastViewport sets it to 1 once the toast has entered', | ||
| default: '0', | ||
| private: true, | ||
| }, | ||
| { | ||
| name: '--_toast-start-slide', | ||
| description: | ||
| 'Private starting offset of the entry; ToastViewport sets it to 0px once the toast has entered', |
There was a problem hiding this comment.
Optional (wording): ToastViewport sets both properties when the toast's entrance starts (its transitionrun), not once it has entered, so these two descriptions could match the source comment you already corrected:
'Private starting opacity of the entry; ToastViewport sets it to 1 once the entrance starts'
'Private starting offset of the entry; ToastViewport sets it to 0px once the entrance starts'There was a problem hiding this comment.
Done at 8bbf387: both descriptions now say "once the entrance starts".
A top-layer ToastViewport moves its host into the latest open modal and back out. Each move renders the toast's row again, and the row's and the card's `@starting-style` played the entrance again. So a toast already on screen faded out and slid in a second time whenever a Dialog, a modal Drawer or a scrim sheet opened or closed over it. A row now keeps its starting style only until its entry transition starts. Then the row drops its closed-track start, and the card's start reads the row's custom properties, so a later move renders the card at rest. A move during a toast's own entrance finishes it at once instead of playing it again. That move cancels the row's entry transition, so no transition ends, and the row settles on the cancel: it releases its clip and the card's shadow paints. AlertDialog's touch exit (meta#8084) keeps the alert open while it fades, so a toast its action raises enters inside the alert and moves out when the fade ends. This fix keeps that toast from entering twice.
797b0b3 to
8bbf387
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. The required checks are waiting for a maintainer to add the ci:run label, so the next step is that label.
The fix restores component:Toast FR5 and FR12: the new browser spec's three move cases fail on main and pass on this branch in Chromium and WebKit, also when merged with current main, and a Dialog, modal Drawer or scrim BottomSheet opening or closing over a toast no longer replays its entrance.
[Automated review]
cixzhang
added
the
ci:run
There was a problem hiding this comment.
Approved.
The fix restores a single entrance for a toast whose viewport moves into or out of a Dialog, a modal Drawer or a scrim BottomSheet (component:Toast FR5 and FR12, spec:AST-003 FR19). The new browser spec's three move cases fail on main and pass on this branch in Chromium and WebKit, the merge of main leaves the fix unchanged, and CI passes on this head.
[Automated review]
User impact
When a
Dialog, a modalDraweror a bottom sheet with a scrim opens or closes while a toast shows, the toast fades out and slides in a second time. On a phone it reads as a flicker: the toast settles, jumps back to its entry offset, and enters again. Main does this on every open or close of such a modal over a toast. AlertDialog's touch exit (#8084) would also do it on its main path: the alert stays open while it fades, so a toast its action raises enters inside the alert and moves out when the fade ends.Problem and solution fit
ToastViewportmoves its host element into the latest open native modal and back out. Moving a node renders it again. The toast row's and the card's entrance are CSS@starting-stylerules, which apply to every render of an element that had no style before, so each move plays the entrance again.The fix keeps the starting style only for a toast's first render, so a move renders the toast at rest.
Expected behavior and authority
component:ToastFR5: "A new card slides in from its edge and fades in".component:ToastFR12: a top-layer viewport moves into the latest open modal and back, "keeping each toast, its timer, and focus inside the viewport".The record already says this, so it does not change.
Smallest restoration
transitionrunforgrid-template-rows, or its settle). Then the row drops its closed-track start, and it sets two custom properties that the card's starting style reads, so a later render starts the card at rest.Toastcard, outside a viewport row, keeps today's starting style through the custom properties' fallbacks.The layer runtime's
preserveAnimationscarries animations across a display reset, but it cannot serve this move for two reasons. It cannot tell a toast whose entrance has not started from one that settled long ago: neither has an animation to save, so it would either drop a first entrance or play a settled one's again. Only the row knows that its entry has started. And it resumes a cancelled transition as a script animation, which fires notransitionend, while the row's settle and its exit both wait for that event.Evidence
time ms: opacity / slide px, D inside the dialog, R at the root) on main's "App toast over dialog" story in Chromium. Closing the dialog over a toast that is still entering:31:0/8D … 131:1/0D 148:0/8R … 248:1/0R. Over a toast that has settled:14:1/0D 31:0/8R … 147:1/0R. WebKit shows the same.Toast/__tests__/ToastViewport.hostMove.interaction.browser.spec.tsrecords the card on each frame: its opacity, where it is (in the dialog or the page), and its opacity transitions. Its cases are "a new toast plays its entrance once", "a toast moved during its entrance finishes it once and settles" (with a slowed entrance, so the move lands inside it), "a toast already shown stays still when the Dialog that hosts it closes" and "… when a Dialog opens over it". Each move case first checks that the card was recorded on both sides of the move, and that the row is shown with a height after it.evidence/c18-toast-alert-pathbranch holds the same recorder for AlertDialog's "Alert over a toast" story (a finger taps Delete, and the alert fades out over the toast it raised). On a build of feat(AlertDialog): under a finger the alert appears and leaves the way iOS's does #8084 at 62c9b4a it fails three runs out of three in Chromium and in WebKit (the toast fades back to 0 and enters again). On feat(AlertDialog): under a finger the alert appears and leaves the way iOS's does #8084 merged with this head it passes three out of three in each. The toast keeps its own pixels while the alert fades: the alert's opacity does not reach it in either engine, so no second entrance and no dimming remain.Scope
Testing
Local gates at 8bbf387, on main 8fdf6be:
CI=true pnpm lint(no errors), the build,pnpm typecheck, Storybook,pnpm test:ui(21,134),pnpm test:node(16,655),check:knowledge,check:repoandspecs:for --diffpass.api:report --checkreports drift already on main (none of it in Toast).